[SYSTEMDS-3955] Add signed SSL certificates support by gaturchenko · Pull Request #2569 · apache/systemds

gaturchenko · 2026-07-29T13:38:46Z

This PR adds support for reading in a signed certificate and makes the coordinator verify it in place of the existing self-signed certificate generation. To that end, the XML config is extended with the following:

Key	Side	Comment
sysds.federated.ssl.cert	worker	X.509 certificate chain, PEM, leaf first
sysds.federated.ssl.key	worker	matching private key, PKCS#8 PEM
sysds.federated.ssl.keyPassword	worker	read from `SYSTEMDS_FEDERATED_SSL_KEY_PASSWORD` env var
sysds.federated.ssl.trust	coordinator	certificates trusted to sign worker certificates, typically the CA

Both certificate and key are read by each worker from its own config, so they are local per worker. The coordinator has a single trust file, where one CA certificate covers any number of workers, and multiple CAs can be concatenated into one PEM. With hostname verification on, a worker's certificate must be issued for the address the script connects to (san=dns:..., or san=ip:... for IP literals), so a certificate valid for one worker cannot be replayed for another.

NB: private keys and certificates are added as a part of this PR for SSL-enabled tests to work. They are generated with a dedicated shell script and are irrelevant for anything except the tests.

codecov · 2026-07-31T10:24:21Z

Codecov Report

❌ Patch coverage is 81.70732% with 15 lines in your changes missing coverage. Please review.
✅ Project coverage is 71.45%. Comparing base (fa9824c) to head (104709d).
⚠️ Report is 5 commits behind head on main.

Files with missing lines	Patch %	Lines
...ime/controlprogram/federated/FederatedSSLUtil.java	78.94%	3 Missing and 5 partials ⚠️
...untime/controlprogram/federated/FederatedData.java	76.47%	2 Missing and 2 partials ⚠️
...time/controlprogram/federated/FederatedWorker.java	87.50%	1 Missing and 2 partials ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #2569      +/-   ##
============================================
- Coverage     71.60%   71.45%   -0.15%     
- Complexity    50259    50413     +154     
============================================
  Files          1623     1629       +6     
  Lines        194314   195334    +1020     
  Branches      37965    38075     +110     
============================================
+ Hits         139130   139576     +446     
- Misses        44277    44831     +554     
- Partials      10907    10927      +20

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

ywcb00

Thank you very much for this PR @gaturchenko. :)
Here a few general comments / questions for my understanding:

Please do not include the key password in the XML config file. Instead, use an environment variable for the key password to avoid leaking this secret when publishing the configuration.
Regarding the configurable hostname verification: In which case do we not want to check whether the hostname matches the certificate? Should we remove this configuration to make the hostname verification mandatory whenever ssl is enabled?

All the best,
David

ywcb00 · 2026-07-31T12:06:41Z

 		}

-		public void setPromise(Promise<FederatedResponse> prom) {
+		public synchronized void setPromise(Promise<FederatedResponse> prom) {


Use synchronized{ } blocks to mark specific critical sections of the code instead of declaring the whole method as 'synchronized'.

gaturchenko · 2026-07-31T14:42:34Z

Thank you very much for this PR @gaturchenko. :) Here a few general comments / questions for my understanding:

Please do not include the key password in the XML config file. Instead, use an environment variable for the key password to avoid leaking this secret when publishing the configuration.

Regarding the configurable hostname verification: In which case do we not want to check whether the hostname matches the certificate? Should we remove this configuration to make the hostname verification mandatory whenever ssl is enabled?

All the best, David

Hey @ywcb00, thanks for the review. Regarding the first point, implemented the change as suggested. About the second, I think the only case is SSH port forwarding, but I think we can remove this option, which I did. Let me know if you have any further comments.

ywcb00

Thank you very much @gaturchenko. :)
I left again some minor comments in the code, could you please have a look? Other than that, the PR looks good to me.
All the best, David

PS: Please do not overwrite the past commits. Instead, just append a new commit to the PR with the changes. This allows for reviewers to only review the new commits instead of reviewing all changes again. Thanks.

ywcb00 · 2026-08-04T08:20:02Z

+    <sysds.federated.ssl.cert>/path/to/worker-cert.pem</sysds.federated.ssl.cert>
+    <sysds.federated.ssl.key>/path/to/worker-key.pem</sysds.federated.ssl.key>
+    <sysds.federated.ssl.trust>/path/to/ca-cert.pem</sysds.federated.ssl.trust>


Since these configurations are required when ssl is enabled, I would include the configurations in this xml file. I see the point that we do not have any certificates for the corresponding tutorial yet, leaving us with three options to resolve this issue:

Generate respective certificates for the tutorial and include them in the repository.

Include the configurations with the current placeholder value (path/to/...) and ensure that, when executing the tutorial with ssl, it is clear from the error messages that these certificates are missing.

Remove the 'ssl' option from scripts/tutorials/federated/parameters.sh.
I think the first option would be the best, and the second option fits the scope of this PR best.

Done (option 2)

ywcb00 · 2026-08-04T08:40:00Z

+		// The promise is assigned by the requesting thread, while the channel events below are handled on the
+		// event loop, and the two orders are not guaranteed: a rejected SSL handshake already fails the channel
+		// while the requesting thread is still connecting.
+		private final Object _promLock = new Object();


Is there a specific reason for creating an object for locking the member variable _prom? I think that we can just use the intrinsic lock from the member variable _prom directly, and thus synchronize on _prom instead of _promLock.

Done (can't lock on _prom but can lock on the class itself via this)

ywcb00 · 2026-08-04T08:55:11Z

+
 			try {
-				context = SslContextBuilder.forClient().trustManager(InsecureTrustManagerFactory.INSTANCE).build();
+				LOG.info("Federated SSL trusting certificates in: " + trustPath);


This LOG should be at debug level

ywcb00 · 2026-08-04T09:05:34Z

+		// The SSL context of the coordinator is cached for the JVM, so it should be cleared in between the tests
+		FederatedSSLUtil.resetClientContext();
+		FederatedData.resetFederatedSites();


Can we move this code to the setUp method to only have one method that is executed before the tests?

…ederated tutorial with SSL

…Lock`

github-project-automation Bot added this to SystemDS PR Queue Jul 29, 2026

github-project-automation Bot moved this to In Progress in SystemDS PR Queue Jul 29, 2026

gaturchenko changed the title ~~feat(federated): add signed SSL certificates support~~ [SYSTEMDS-3955] Add signed SSL certificates support Jul 29, 2026

ywcb00 requested changes Jul 31, 2026

View reviewed changes

github-project-automation Bot moved this from In Progress to In Review in SystemDS PR Queue Jul 31, 2026

feat(federated): add signed SSL certificates support

9fe0df9

gaturchenko force-pushed the fed-ssl branch from d553350 to 9fe0df9 Compare July 31, 2026 14:34

ywcb00 self-assigned this Aug 4, 2026

ywcb00 requested changes Aug 4, 2026

View reviewed changes

gaturchenko added 4 commits August 4, 2026 13:37

feat(tutorials): inform about missing certificates when running the f…

e555b6f

…ederated tutorial with SSL

fix(federated): change log level from info to debug for FederatedSSLUtil

11031bc

fix(tests): reset SSL context in setUp for FederatedSSLTest

a3dbbfa

fix(federated): lock on DataRequestHandler itself instead of `_prom…

104709d

…Lock`

ywcb00 closed this Aug 4, 2026

github-project-automation Bot moved this from In Review to Done in SystemDS PR Queue Aug 4, 2026

Uh oh!

Conversation

gaturchenko commented Jul 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Jul 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

ywcb00 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gaturchenko commented Jul 31, 2026

Uh oh!

ywcb00 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

gaturchenko commented Jul 29, 2026 •

edited

Loading

codecov Bot commented Jul 31, 2026 •

edited

Loading